Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix suggestion for removing &mut from &mut macro!(). #85939

Merged
merged 2 commits into from
Jun 5, 2021
Merged

Fix suggestion for removing &mut from &mut macro!(). #85939

merged 2 commits into from
Jun 5, 2021

Conversation

m-ou-se
Copy link
Member

@m-ou-se m-ou-se commented Jun 2, 2021

Fixes #85933

Before: (Note the suggestions.)

error[E0308]: mismatched types
 --> src/main.rs:2:21
  |
2 |     let _: String = &mut format!("");
  |            ------   ^^^^^^^^^^^^^^^^
  |            |        |
  |            |        expected struct `String`, found `&mut String`
  |            |        help: consider removing the borrow: `mut format!("")`
  |            expected due to this

error[E0308]: mismatched types
 --> src/main.rs:3:21
  |
3 |     let _: String = &mut (format!(""));
  |            ------   ^^^^^^^^^^^^^^^^^^
  |            |        |
  |            |        expected struct `String`, found `&mut String`
  |            |        help: consider removing the borrow: `mut (format!(""))`
  |            expected due to this

After:

error[E0308]: mismatched types
 --> src/main.rs:2:21
  |
2 |     let _: String = &mut format!("");
  |            ------   ^^^^^^^^^^^^^^^^
  |            |        |
  |            |        expected struct `String`, found `&mut String`
  |            |        help: consider removing the borrow: `format!("")`
  |            expected due to this

error[E0308]: mismatched types
 --> src/main.rs:3:21
  |
3 |     let _: String = &mut (format!(""));
  |            ------   ^^^^^^^^^^^^^^^^^^
  |            |        |
  |            |        expected struct `String`, found `&mut String`
  |            |        help: consider removing the borrow: `format!("")`
  |            expected due to this

@m-ou-se m-ou-se requested a review from estebank June 2, 2021 17:09
@rust-highfive
Copy link
Collaborator

r? @jackh726

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 2, 2021
m-ou-se
m-ou-se commented Jun 2, 2021
View reviewed changes
compiler/rustc_typeck/src/check/demand.rs
// E.g. for `&format!("")`, where we want the span to the
// `format!()` invocation instead of its expansion.
if let Some(call_span) =
iter::successors(Some(expr.span), |s| s.parent()).find(|&s| sp.contains(s))
Copy link
Member Author

@m-ou-se m-ou-se Jun 2, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@estebank What do you think about adding this as a function on Span (e.g. span.find_parent_within(span) or something)?

I have a feeling more diagnostics can/should use this, but I haven't written enough diagnostics yet to know if that's true.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with your assessment and we should have something like that (bikeshed notwithstanding).

m-ou-se
m-ou-se commented Jun 2, 2021
View reviewed changes
compiler/rustc_typeck/src/check/demand.rs
Comment on lines -585 to +593
src.to_string(),
code,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In another diagnostic, I emitted (potentially multiple) suggestions for removing the parts that need to be removed, instead of a single suggestion that contains a copy of (part of) the original source. Is there a guideline for which to prefer?

Basically:
A. "replace &mut (format!("")) by format!("")
B. "remove &mut ( and remove )"

Most diagnostics seem to use A, which surprises me a bit. The suggestion for A can get quite large for large expressions.

Copy link
Contributor

@estebank estebank Jun 3, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The later didn't exist until "recently" (2019?), so there's a historical component. There's also a bug that needs to be fixed on rustc's side to allow rustfix apply multipart suggestions, which it can't today.

Even with that caveat, I push for multipart suggestions in all new diagnostics when possible/necessary.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

There's also a bug that needs to be fixed on rustc's side to allow rustfix apply multipart suggestions, which it can't today.

That should be fixed by rust-lang/rustfix#195

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome! That should also allow us to add a // run-rustfix to a bunch of tests that don't have it today because of that 😄

@estebank
Copy link
Contributor

estebank commented Jun 3, 2021

@bors r+, but feel free to change the suggestion to be multipart, if you so desire.

@estebank
Copy link
Contributor

estebank commented Jun 3, 2021

r? @estebank

@rust-highfive rust-highfive assigned estebank and unassigned jackh726 Jun 3, 2021
@m-ou-se
Copy link
Member Author

m-ou-se commented Jun 4, 2021

@bors r+, but feel free to change the suggestion to be multipart, if you so desire.

Might do that in another PR. :)

Hm, looks like bors didn't understand r+,

@bors r=estebank

@bors
Copy link
Contributor

bors commented Jun 4, 2021

📌 Commit ecebb66 has been approved by estebank

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 4, 2021
@JohnTitor JohnTitor mentioned this pull request Jun 4, 2021
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 4, 2021
@bors
Auto merge of rust-lang#86006 - JohnTitor:rollup-97iuoi3, r=JohnTitor
704934d 
Rollup of 10 pull requests

Successful merges:

 - rust-lang#83653 (Remove unused code from `rustc_data_structures::sync`)
 - rust-lang#84466 (rustdoc: Remove `PrimitiveType::{to_url_str, as_str}`)
 - rust-lang#84880 (Make match in `register_res` easier to read)
 - rust-lang#84942 (rustdoc: link to stable/beta docs consistently in documentation)
 - rust-lang#85853 (Warn against boxed DST in `improper_ctypes_definitions` lint)
 - rust-lang#85939 (Fix suggestion for removing &mut from &mut macro!().)
 - rust-lang#85966 (wasm: Make simd types passed via indirection again)
 - rust-lang#85979 (don't suggest unsized indirection in where-clauses)
 - rust-lang#85983 (Update to semver 1.0.3)
 - rust-lang#85988 (Note that `ninja = false` goes under `[llvm]`)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 5ebc4d3 into rust-lang:master Jun 5, 2021
@rustbot rustbot added this to the 1.54.0 milestone Jun 5, 2021
@m-ou-se m-ou-se deleted the fix-remove-ref-macro-invocation branch June 14, 2021 20:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@estebank estebank Awaiting requested review from estebank

Assignees

@estebank estebank

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
1.54.0
Development

Successfully merging this pull request may close these issues.

"consider removing the borrow" suggests to only remove & and not mut
6 participants
@m-ou-se @rust-highfive @estebank @bors @jackh726 @rustbot